Cut 0.0.107 #1525

jkczyz · 2022-06-06T18:53:15Z

No description provided.

jkczyz · 2022-06-06T18:53:26Z

Waiting on #1517.

CHANGELOG.md

TheBlueMatt · 2022-06-07T10:51:34Z

CHANGELOG.md

+   overwhelming default logging (#1421).
+ * `PeerManager` supports processing messages from different peers in parallel
+   (#1023).
+ * `PeerManager` uses a single `Secp256k1` across all peers (#1472).


Is this a perf improvement? I guess it's a memory usage thing but we should specify what impact things have, not what we did to get it.

Updated but let me know if you prefer to drop it.

Maybe drop the how? Just say that we've reduced memory usage used per-peer in PeerManager?

CHANGELOG.md

TheBlueMatt · 2022-06-07T10:55:34Z

CHANGELOG.md

+ * `BackgroundProcessor` process pending events before processing network
+   messages to reduce lock contention in `ChannelManager`, which can be
+   problematic for extremely slow machines (#1436).
+ * Fixed an integer underflow during channel opening occurring when the channel


Did this actually change --release code? IIRC it does not have an impact, just corrects the error message - not sure it's worth even mentioning.

CHANGELOG.md

TheBlueMatt · 2022-06-07T11:08:51Z

CHANGELOG.md

+   been renamed `NetworkUpdate::ChannelFailure` (#1159).
+ * Added a `filtered_block_connected` method to `chain::Listen` and a default
+   implementation of `block_connected` in order to support BIP 157/158 compact
+   block filters (#1453).


I don't think it's useful for compact block filters cause in that case you have the full block. Maybe just say "for those fetching filtered instead of full blocks"?

Changed though note for compact block filters you still need to connect empty blocks if the filter doesn't match.

jkczyz · 2022-06-07T16:42:07Z

Rebased to resolve a merge conflict in a Cargo file.

devrandom · 2022-06-07T19:50:26Z

Should mention greatly reduced memory usage due to rust-bitcoin / libsecp upgrade.

ViktorTigerstrom · 2022-06-07T19:51:21Z

CHANGELOG.md

+   `chainmonitor::Persist` are given for types implementing `KVStorePersister`.
+   ` lightning-persister`'s `FilesystemPersister` implements `KVStorePersister`
+   (#1417).
+ * `ChannelDetails` and `ChannelCounterparty` include fields for HTLC minimum


Not sure how detailed we want to be in these release notes, but here are two points that may be worth adding about #1378 and #1444:

With Include htlc min/max for ChannelDetails and ChannelCounterparty #1378 the lightning-invoice crate now sets a value for htlc_minimum_msat and htlc_maximum_msat in the invoice RouteHints if such values exists.

Set ChannelUpdate htlc_maximum_msat using the peer's value #1444 added a new config field max_inbound_htlc_value_in_flight_percent_of_channel that allows the user to specify the percentage of the channel value which the total value of outstanding inbound HTLCs for the channel will be capped to.

Yea, 1444 is unrelated here - it's a bugfix to set the value correctly. The new config knob is in the next bullet.

Yea, 1444 is unrelated here - it's a bugfix to set the value correctly. The new config knob is in the next bullet.

Nice, thanks for adding the config knob as a separate bullet @jkczyz. This is now resolved after 1444 is removed from this bullet (+ the comment about the BOLT11 hints encoding below) :)

dunxen

ACK 6386716

dunxen · 2022-06-07T19:56:25Z

CHANGELOG.md

+   and maximum values (#1378, #1444).
+ * `ProabilisticScorer` is parameterized by a `Logger`, which it uses to log
+   channel liquidity updates or lack thereof (#1405).
+ * `ChannelDetails` has a `outbound_htlc_limit_msat` field, which should be used


Nit

Suggested change

* `ChannelDetails` has a `outbound_htlc_limit_msat` field, which should be used

* `ChannelDetails` has an `outbound_htlc_limit_msat` field, which should be used

dunxen · 2022-06-07T19:58:43Z

CHANGELOG.md

+   unconfirmed rather than as it loses confirmations (#1461).
+ * Fixed a panic in `lightning-net-tokio` when fetching a peer's socket address
+   after the connection has been closed (#1449).
+ * `find_route` will no longer return routes that would cause Onion construction


nit: common noun?

Suggested change

* `find_route` will no longer return routes that would cause Onion construction

* `find_route` will no longer return routes that would cause onion construction

Maybe "in some cases"? In the Bug Fixes section I'd prefer we be as exact as possible, especially given we know of other cases where find_route can return routes that channelmanager will refuse lol.

CHANGELOG.md

TheBlueMatt · 2022-06-07T20:25:47Z

CHANGELOG.md

+# 0.0.107 - 2022-06-07
+
+## API Updates
+ * Wumbo channels are now supported and can be enabled for inbound channels


Is this really the most important API change we have in 107?

Hmmm... not sure. Figured given everyone else has supported it for ages, it's quite notable that we now do, too. Happy to push this down if you prefer.

TheBlueMatt · 2022-06-07T20:29:39Z

CHANGELOG.md

+ * Upgraded to `bitcoin` crate version 0.28.1 (#1389).
+ * `ShutdownScript::new_witness_program` now takes a `WitnessVersion` instead of
+   a `NonZeroU8` (#1389).
+ * When a `channel_update` message is included in an onion error's `failuremsg`,


I don't think this is (really) an API change, more a bug fix or spec compliance, if its even worth documenting, but I think the bigger change is the bugfix where we support reading such messages, less generating them.

Placed it in the spec compliance section and expanded to include reading. Let me know if you prefer the latter go under bug fixes.

CHANGELOG.md

TheBlueMatt · 2022-06-07T20:39:20Z

CHANGELOG.md

+   unconfirmed rather than as it loses confirmations (#1461).
+ * Fixed a panic in `lightning-net-tokio` when fetching a peer's socket address
+   after the connection has been closed (#1449).
+ * `find_route` will no longer return routes that would cause Onion construction


Maybe "in some cases"? In the Bug Fixes section I'd prefer we be as exact as possible, especially given we know of other cases where find_route can return routes that channelmanager will refuse lol.

TheBlueMatt · 2022-06-07T20:40:17Z

CHANGELOG.md

+   overwhelming default logging (#1421).
+ * `PeerManager` supports processing messages from different peers in parallel
+   (#1023).
+ * `PeerManager` uses a single `Secp256k1` across all peers (#1472).


Maybe drop the how? Just say that we've reduced memory usage used per-peer in PeerManager?

CHANGELOG.md

TheBlueMatt · 2022-06-07T20:43:59Z

CHANGELOG.md

+   which can be determined at runtime (#1423).
+ * `lightning-invoice` crate's `utils` now accept an expiration time (#1422,
+   #1474).
+ * `find_route` now assumes variable-length onions by default (#1414).


something about "nodes supporting" or something, as a user its entirely unclear to me what "variable-length onions" means or why find_route is assuming it, and where. Maybe we need a "spec compliance" "peer behavior changes" section to separate this out from developer-visible API changes? That would also include things like the warn-on-stale-peer, the channel_update failuremsg stuff, etc.

Or another organization could be to document API breaking changes where we expect the user to take actions on from API extensions where we add new features and the user can stay passive? What are our users release deployment flow and which informations would they like to be prioritized ?

Made a spec compliance section.

TheBlueMatt · 2022-06-07T20:45:11Z

CHANGELOG.md

+* `ChannelManager` serialization is no longer compatible with versions prior to
+  0.0.99 (#1401).
+
+In total, this release features 96 files changed, 9304 insertions, 4503


Phew this was a big release. API-change-wise too. Need to be more aggressive about shipping releases more often, IMO.

ariard

Nice if we document the new features in the website

ariard · 2022-06-07T23:44:58Z

CHANGELOG.md

+## API Updates
+ * Wumbo channels are now supported and can be enabled for inbound channels
+   using `ChannelHandshakeLimits::max_funding_satoshis` (#1425).
+ * Support for feature `option_zeroconf`, allowing immediate forwarding of


Would be nice to document "zero-conf" in the website, maybe in a "Supported Features" subsection.

ariard · 2022-06-07T23:50:13Z

CHANGELOG.md

+ * `ChannelManager::claim_funds` no longer returns a `bool` to indicate success.
+   Instead, an `Event::PaymentClaimed` is generated if the claim was successful.
+   Likewise, `ChannelManager::fail_htlc_backwards` no longer has a return value
+   (#1434).


The commit message of this specific change is saying "We can revisit this decision based on user feedback, but will need to very carefully document the potential failure modes here if we do", should we mark more explicitly as waiting feedback on that change ? Not sure folks are gonna read commit messages.

I'm more inclined to keep this brief and allow users to complain if they find it confusing.

ariard · 2022-06-07T23:56:18Z

CHANGELOG.md

+   Likewise, `ChannelManager::fail_htlc_backwards` no longer has a return value
+   (#1434).
+ * `lightning-rapid-gossip-sync` is a new crate for syncing gossip data from a
+   server, primarily aimed at mobile devices (#1155).


Same, would be nice to document it in the website. The README.md is quite nice though not sure it's enough to guide a mobile dev to deploy a gossip sync server and integrate it in its own backend.

ariard · 2022-06-08T00:00:05Z

CHANGELOG.md

+   which can be determined at runtime (#1423).
+ * `lightning-invoice` crate's `utils` now accept an expiration time (#1422,
+   #1474).
+ * `find_route` now assumes variable-length onions by default (#1414).


Or another organization could be to document API breaking changes where we expect the user to take actions on from API extensions where we add new features and the user can stay passive? What are our users release deployment flow and which informations would they like to be prioritized ?

CHANGELOG.md

ariard · 2022-06-08T00:06:17Z

CHANGELOG.md

+   `debug_log_liquidity_stats` (#1460).
+ * `BackgroundProcessor` now takes an optional `WriteableScore` which it will
+   persist using the `Persister` trait's new `persist_scorer` method (#1416).
+ * A `warn` message is now sent when receiving a `channel_reestablish` with an


That type of change is nice to know if you're connecting with another implementation or old version but you don't really expect the user to change its application in consequence. At least it's a p2p relaxation here.

Moved to new spec section.

ariard · 2022-06-08T00:08:48Z

CHANGELOG.md

+   identifiers have been renamed accordingly (#1506).
+ * `core2::io` or `std::io` (depending on feature flags `no-std` or `std`) is
+   exported as a `lightning::io` module (#1504).
+ * The deprecated `Scorer` has been removed in favor or `ProbabilisitcScorer`


s/ProbabilisitcScorer/ProbabilisticScorer/g

Nit: this typo still exists in the latest diff.

ariard · 2022-06-08T00:29:15Z

CHANGELOG.md

+ * `PeerManager` uses a single `Secp256k1` across all peers for reduced memory
+   usage (#1472).
+
+## Bug Fixes


I think it misses #1454 ?

And the sister PR #1463

Dropped in the fixup as per 483dd8e#r891072611

jkczyz · 2022-06-08T04:53:32Z

Nice if we document the new features in the website

Yeah, definitely need to improve that. We've been working on some visuals for a presentation that could be repurposed.

CHANGELOG.md

TheBlueMatt

Almost good. I'm happy to see this squashed regularly, dunno about other reviewers.

CHANGELOG.md

TheBlueMatt · 2022-06-08T08:20:57Z

CHANGELOG.md

+   `chainmonitor::Persist` are given for types implementing `KVStorePersister`.
+   ` lightning-persister`'s `FilesystemPersister` implements `KVStorePersister`
+   (#1417).
+ * `ChannelDetails` and `ChannelCounterparty` include fields for HTLC minimum


Note that these values are not encoded in the BOLT 11 hints, it's more of a forward compat thing. Don't think it's worth mentioning anything in the invoice here.

TheBlueMatt · 2022-06-08T08:21:35Z

CHANGELOG.md

+   `chainmonitor::Persist` are given for types implementing `KVStorePersister`.
+   ` lightning-persister`'s `FilesystemPersister` implements `KVStorePersister`
+   (#1417).
+ * `ChannelDetails` and `ChannelCounterparty` include fields for HTLC minimum


Yea, 1444 is unrelated here - it's a bugfix to set the value correctly. The new config knob is in the next bullet.

TheBlueMatt · 2022-06-08T08:31:27Z

CHANGELOG.md

+
+## Spec Compliance
+ * `find_route` now assumes variable-length onions by default for nodes
+   supporting the feature (#1414).


"Assumes.... for nodes supporting this feature" makes no sense. We assume it when we don't know if the node supports this feature now.

Ah, misinterpreted your earlier comment.

CHANGELOG.md

devrandom · 2022-06-08T10:01:32Z

Should mention greatly reduced memory usage due to rust-bitcoin / libsecp upgrade.

bump

TheBlueMatt · 2022-06-08T10:34:59Z

Should mention greatly reduced memory usage due to rust-bitcoin / libsecp upgrade.

What exactly should we write for this?

devrandom · 2022-06-08T10:48:20Z

Should mention greatly reduced memory usage due to rust-bitcoin / libsecp upgrade.

What exactly should we write for this?

greatly reduced per-channel and per-node memory usage, due to upgrade of rust-secp256k1 to 0.22.1 / rust-bitcoin to 0.28.1

TheBlueMatt · 2022-06-08T11:04:59Z

Lol easy enough.

codecov-commenter · 2022-06-08T12:45:09Z

Codecov Report

Merging #1525 (b2e635f) into main (a551a62) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1525      +/-   ##
==========================================
+ Coverage   90.94%   90.95%   +0.01%     
==========================================
  Files          80       80              
  Lines       43426    43426              
  Branches    43426    43426              
==========================================
+ Hits        39492    39497       +5     
+ Misses       3934     3929       -5

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`97.10% <0.00%> (+0.04%)`	⬆️
lightning/src/ln/peer_channel_encryptor.rs	`94.73% <0.00%> (+0.26%)`	⬆️
lightning/src/util/events.rs	`41.98% <0.00%> (+0.32%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a551a62...b2e635f. Read the comment docs.

jkczyz · 2022-06-08T12:45:24Z

Squashed! Let me know if I missed anything.

TheBlueMatt

LGTM with two notes about backwards compat.

TheBlueMatt · 2022-06-08T13:30:14Z

CHANGELOG.md

+ * `ProbabilisticScorer` uses more precision when approximating `log10` (#1406).
+
+## Serialization Compatibility
+* `ChannelManager` serialization is no longer compatible with versions prior to


We should also mention that channels with the zero Conf feature enabled (not required for 0conf channel use) will be unreadable by versions prior to 107.

Is this for both ChannelManager::accept_inbound_channel_from_trusted_peer_0conf and ChannelHandshakeLimits::trust_own_funding_0conf? Should this be called out in their respective docs?

Might have misread this. Was assuming you meant whenever Channel::set_0conf. Was this comment specific about features set on the channel? Let me know if there's a phrasing that you would prefer.

It's kinda independent - we never set the feature, but still do 0conf. Inbound channel peers may set the feature, which would cause incompatibility.

It's called out in inbound channel event docs.

It's kinda independent - we never set the feature, but still do 0conf. Inbound channel peers may set the feature, which would cause incompatibility.

Don't we set as optional in InitFeatures::known?

BTW, pushed wording essentially what you said in the first comment. Let me know if you prefer different wording.

Right, its presence in the init/node features is pretty different from it in channel_type.

CHANGELOG.md

TheBlueMatt · 2022-06-08T16:29:07Z

CHANGELOG.md

@@ -82,6 +230,9 @@
   path with an overall lower cost (#1399).

 ## Serialization Compatibility
+ * All above new events/fields are ignored by prior clients. All above new


Hmm, did we have any new fields that this apply to in 106? I'm not sure that we did. Same goes for 105.

Oops, removed.

wpaulino · 2022-06-08T22:55:59Z

CHANGELOG.md

+   identifiers have been renamed accordingly (#1506).
+ * `core2::io` or `std::io` (depending on feature flags `no-std` or `std`) is
+   exported as a `lightning::io` module (#1504).
+ * The deprecated `Scorer` has been removed in favor or `ProbabilisitcScorer`


Nit: this typo still exists in the latest diff.

ariard · 2022-06-08T23:49:38Z

Sounds mentioning #1454 / #1463 in bug fix has been lost : #1525 (comment) ?

TheBlueMatt · 2022-06-09T00:00:52Z

Jeff responded at #1525 (comment) with a link that github has now eaten and decided we dont need to see :(. But the context there was I'd suggested dropping it since it just provides a better error message for otherwise-unusable channels. More of a "better error for a case you should never hit" which I'm not sure we need to document. Happy to revisit in a new PR editing CHANGELOG.

jkczyz · 2022-06-09T00:03:07Z

Jeff responded at #1525 (comment) with a link that github has now eaten and decided we dont need to see :(. But the context there was I'd suggested dropping it since it just provides a better error message for otherwise-unusable channels. More of a "better error for a case you should never hit" which I'm not sure we need to document. Happy to revisit in a new PR editing CHANGELOG.

FYI, here's a workable link to the discusison: #1525 (comment)

jkczyz changed the title ~~Cut 0.0.106~~ Cut 0.0.107 Jun 6, 2022

jkczyz added this to the 0.0.107 milestone Jun 6, 2022

TheBlueMatt reviewed Jun 7, 2022

View reviewed changes

jkczyz force-pushed the 2022-05-release-0.0.107 branch 2 times, most recently from 7913f17 to 6386716 Compare June 7, 2022 16:40

TheBlueMatt added the Seeking Code Review label Jun 7, 2022

ViktorTigerstrom reviewed Jun 7, 2022

View reviewed changes

dunxen previously approved these changes Jun 7, 2022

View reviewed changes

TheBlueMatt reviewed Jun 7, 2022

View reviewed changes

ariard reviewed Jun 8, 2022

View reviewed changes

Add lightning-rapid-gossip-sync to README

aa1de2d

jkczyz dismissed dunxen’s stale review via a2d78a9 June 8, 2022 04:52

jkczyz force-pushed the 2022-05-release-0.0.107 branch from 6386716 to a2d78a9 Compare June 8, 2022 04:52

tnull reviewed Jun 8, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Show resolved Hide resolved

TheBlueMatt reviewed Jun 8, 2022

View reviewed changes

jkczyz force-pushed the 2022-05-release-0.0.107 branch from a2d78a9 to 9d11374 Compare June 8, 2022 12:44

devrandom previously approved these changes Jun 8, 2022

View reviewed changes

TheBlueMatt reviewed Jun 8, 2022

View reviewed changes

jkczyz dismissed devrandom’s stale review via e7e539e June 8, 2022 15:05

jkczyz force-pushed the 2022-05-release-0.0.107 branch from 9d11374 to e7e539e Compare June 8, 2022 15:05

TheBlueMatt reviewed Jun 8, 2022

View reviewed changes

jkczyz force-pushed the 2022-05-release-0.0.107 branch from e7e539e to a66b206 Compare June 8, 2022 17:04

TheBlueMatt previously approved these changes Jun 8, 2022

View reviewed changes

wpaulino previously approved these changes Jun 8, 2022

View reviewed changes

jkczyz added 2 commits June 8, 2022 18:16

Update CHANGELOG for 0.0.107

e190242

Bump crate versions to 0.0.107/invoice 0.15

b2e635f

jkczyz dismissed stale reviews from wpaulino and TheBlueMatt via b2e635f June 8, 2022 23:17

jkczyz force-pushed the 2022-05-release-0.0.107 branch from a66b206 to b2e635f Compare June 8, 2022 23:17

TheBlueMatt approved these changes Jun 8, 2022

View reviewed changes

wpaulino approved these changes Jun 8, 2022

View reviewed changes

TheBlueMatt merged commit 5c788a0 into lightningdevkit:main Jun 8, 2022

	* `ChannelDetails` has a `outbound_htlc_limit_msat` field, which should be used
	* `ChannelDetails` has an `outbound_htlc_limit_msat` field, which should be used

	* `find_route` will no longer return routes that would cause Onion construction
	* `find_route` will no longer return routes that would cause onion construction

Cut 0.0.107 #1525

Cut 0.0.107 #1525

Conversation

jkczyz commented Jun 6, 2022

jkczyz commented Jun 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz commented Jun 7, 2022

devrandom commented Jun 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

dunxen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz commented Jun 8, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devrandom commented Jun 8, 2022

TheBlueMatt commented Jun 8, 2022

devrandom commented Jun 8, 2022

TheBlueMatt commented Jun 8, 2022

codecov-commenter commented Jun 8, 2022 • edited Loading

Codecov Report

jkczyz commented Jun 8, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard commented Jun 8, 2022

TheBlueMatt commented Jun 9, 2022

jkczyz commented Jun 9, 2022

ViktorTigerstrom Jun 8, 2022 •

edited

Loading

ariard left a comment •

edited

Loading

codecov-commenter commented Jun 8, 2022 •

edited

Loading

jkczyz Jun 8, 2022 •

edited

Loading